Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Play scripts upgrade to Python 3 #1355

Merged
merged 19 commits into from
Apr 3, 2022
Merged

Conversation

tomparle
Copy link
Contributor

Upgrade all Play commands to python 3 (they were originally written in python 2).

Purpose

This allow to keep Play 1 up-to-date with the current status of Python, whose version 2 is now longer maintained since January 1st, 2020.
This also allows to use Play in modern environments without Python 2 support, like Heroku current stack.

Fixes

fixes #1353

Notes

I helped the migration with futurize but there was still quite some work to do manually.
I tested most commands and a successful build to Heroku-20 stack as well (which only supports python 3).

Migration notes

This is a breaking change. This PR removes Python 2 support and requires now Python 3.

You may want to remove some print "hello" lines in some modules of your project that uses Play. The syntax causes an error in Python 3, and those lines are just sample code from the module template. They can be removed safely.
If you have custom-made modules with python scripts, you should migrate them manually to Python 3 (see some hints here : https://docs.python.org/3/howto/pyporting.html)

@tomparle
Copy link
Contributor Author

Update (25 oct 2021)
I had not seen that the build failed because of Python tests.
I have spent several hours to update them because of deadlock issues with sample Play apps not being killed properly, and trying several libs for making tests work with Python 3. I'm glad that tests finally pass.
I also upgraded the yaml lib which could raise some errors without visible impact though.
Hope this will help some people and make this upgrade available to the master branch !

@xael-fry xael-fry self-requested a review January 6, 2022 13:35
@xael-fry
Copy link
Member

The python exec has not been updated (/python), Do we really need to keep it ? I thinking about removing it ?
What do you think ?
@tomparle @asolntsev @tazmaniax ?

@asolntsev
Copy link
Contributor

The python exec has not been updated (/python), Do we really need to keep it ? I thinking about removing it ? What do you think ? @tomparle @asolntsev @tazmaniax ?

I personally haven't used /python for a long time, but how will work users who used to play foo commands?

@tazmaniax
Copy link
Collaborator

I was thinking the same thing. I don't think we can lose the Python command wrapper for the moment and ideally it can be upgraded to support Python 3. I think I'm in the same boat as @tomparle with using Heroku which now only supports Python 3 with its latest stack.

@xael-fry
Copy link
Member

the wrapper is only for windows user, as it is the python.exe. So for windows user they will need to have it on their computer same for linux. or am I missing something ?

@asolntsev
Copy link
Contributor

@xael-fry Ah Windows... Then we can drop it. Who cares about Windows users. :)

@tazmaniax
Copy link
Collaborator

Ah ok, I was confused about what you were referring to. As long as I can still call Play commands on MacOS and Linux then I'm good 😄

@tomparle
Copy link
Contributor Author

tomparle commented Feb 1, 2022

Sorry for the late answer and thank you for the merge !
Indeed, the /python folder does not seem mandatory, but since I do not use Windows I'm not sure 100%.
Thanks for all the feedback.

@aleksandy
Copy link
Contributor

Removing the /python folder will make play! useless out of the box in a Windows environment if the python is not installed. In my opinion, it is very bad for newbies if the getting started tips will fail because of lack of python.

@xael-fry xael-fry self-assigned this Feb 7, 2022
@xael-fry xael-fry added this to the 1.7.0 milestone Feb 7, 2022
xael-fry added a commit that referenced this pull request Feb 7, 2022
xael-fry added a commit that referenced this pull request Feb 7, 2022
xael-fry added a commit to xael-fry/play that referenced this pull request Feb 10, 2022
xael-fry added a commit that referenced this pull request Feb 10, 2022
xael-fry added a commit to xael-fry/play that referenced this pull request Mar 13, 2022
xael-fry added a commit to xael-fry/play that referenced this pull request Mar 13, 2022
xael-fry added a commit to xael-fry/play that referenced this pull request Mar 13, 2022
xael-fry added a commit to xael-fry/play that referenced this pull request Mar 13, 2022
xael-fry added a commit to xael-fry/play that referenced this pull request Mar 13, 2022
xael-fry added a commit that referenced this pull request Mar 13, 2022
xael-fry added a commit that referenced this pull request Mar 13, 2022
xael-fry added a commit that referenced this pull request Mar 13, 2022
@xael-fry
Copy link
Member

@tomparle can you rebase this pr to master ?

@tomparle
Copy link
Contributor Author

@xael-fry I spent quite some time to rebase properly (and fix my previous rebase mistakes), upgrade some recent changes to python 3 syntax, and fix some python libs which I did not setup properly due to some confusion with python system libs.

Finally the merge and the tests seems ok, except for the windows environment test suite which does not terminate. I do not use Windows for developing and does not know what's causing these tests to be suspend so some help could be useful.
Anyway I do not think this should be blocking for this merge.

xael-fry added a commit to xael-fry/play that referenced this pull request Mar 30, 2022
xael-fry added a commit to xael-fry/play that referenced this pull request Mar 30, 2022
xael-fry added a commit to xael-fry/play that referenced this pull request Apr 2, 2022
xael-fry added a commit to xael-fry/play that referenced this pull request Apr 2, 2022
xael-fry added a commit to xael-fry/play that referenced this pull request Apr 2, 2022
xael-fry added a commit to xael-fry/play that referenced this pull request Apr 2, 2022
xael-fry added a commit to xael-fry/play that referenced this pull request Apr 3, 2022
xael-fry added a commit to xael-fry/play that referenced this pull request Apr 3, 2022
xael-fry added a commit to xael-fry/play that referenced this pull request Apr 3, 2022
xael-fry added a commit to xael-fry/play that referenced this pull request Apr 3, 2022
xael-fry added a commit to xael-fry/play that referenced this pull request Apr 3, 2022
xael-fry added a commit to xael-fry/play that referenced this pull request Apr 3, 2022
xael-fry added a commit to xael-fry/play that referenced this pull request Apr 3, 2022
xael-fry added a commit to xael-fry/play that referenced this pull request Apr 3, 2022
xael-fry added a commit that referenced this pull request Apr 3, 2022
@xael-fry xael-fry merged commit 5bd109a into playframework:master Apr 3, 2022
@Alexandermjos
Copy link

Has anyone tried this on Windows? I get "ModuleNotFoundError: No module named 'win32pdh'" when using python 3.9.1 on Windows.
"include win32pdh" works fine in python 2.7.8 included in Play 1.5.2

@rakix
Copy link

rakix commented Sep 7, 2022

$ pip install pypiwin32

works fine Play 1.7 with Windows

@Alexandermjos
Copy link

Thank you 👍
Is this considered common knowledge or something to consider putting into the documentation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

python3 support or workaround
7 participants